Display availability statements when relevant#408
Conversation
Coverage Report for CI Build 27801626200Coverage increased (+0.003%) to 98.356%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
❌ 9 blocking issues (10 total)
|
This will be loaded via Stimulus when needed in a subsequent commit. Alma SRU controller tests
This still needs to be filtered, most likely (tho the AlmaSru model will also do its own filtering)
This involves adding lazy-loading support to the content loader controller, which may be a feature that other async calls would benefit from.
- Also adds additional filter on UI invoking this path, leveraging new validation method
** Why are these changes being introduced: Rather than render multiple availability statements, we want to just show the first, with a suffix phrase of "and other locations" if there are more locations. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-599 ** How does this address that need: This slightly refactors the formatting logic for handling the statements after they've been formatted. All blocks are formatted to start with, and then if there are multiple statements we append the suffix. Only the first statement is ever returned, as a list of one entry - so the overall contract of the .lookup() method is unchanged. ** Document any side effects to this change: It is a bit inefficient to format records that we then discard. I'm not sure that this will be a significant issue, but it should be noted.
** Why are these changes being introduced: Up until now, the availability statements have been empty links - but we want them to go to the full records view, and specifically to the locations block of that page. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-599 ** How does this address that need: This calls the new PrimoLinkBuilder's full_record_link method, and adds the `getit_link1_0` document fragment to send the user to the list of available locations. ** Document any side effects to this change: There are two concerns to note here: 1. The document fragment we are using feels fragile, and like it may go away in the future. I also haven't checked what the fragment is for NDE. 2. This link is _very_ similar to the full record link that appears immediately before this in the UI. It will likely pass an accessibility check because it is not identical - but this feels like a technicality, and not an ideal user experience. I'm choosing to propose this anyway to see if I'm alone in thinking this - I can live with it, but it would not take much for me to support an alternative.
** Why are these changes being introduced: Our previous implementation of the availability statements had some nice features like icons and bold styling for location names. We want to carry this forward, but want to keep the model-level implementation for this formatting. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-599 ** How does this address that need: This removes the availability() and location() record helpers, and reintroduces those features within the AlmaSru model. This includes the status handling logic of the previous implementation. ** Document any side effects to this change: Other parts of the application rely on the icon() helper in the record helper model, so we can't remove that here. As a result, there is now a duplication of that logic in two places in the application, which isn't ideal. Avoiding this would mean changing where the AlmaSru model formats its output, which I'd rather not do at this point - but I'm open to feedback about this during code review.
| else | ||
| Rails.logger.error("Unhandled availability status: #{status}") | ||
| "#{_icon('question')} Uncertain availability (#{status.humanize}) in " | ||
| end |
There was a problem hiding this comment.
This method has one construct - the case statement for building message text based on the item's status. I'm not sure how this can be made shorter, except for maybe dropping the error handling for the unexpected branch - but I'm not certain that's a good idea.
I am open to another way of handling an unexpected status - maybe a Sentry exception would be more appropriate - but this would still result in a method of the same length.
|
|
||
| assert_equal(['Available at Rotch Library Stacks (NA680.C25 2007)'], result) | ||
| assert_equal( | ||
| ["<i class='fa-sharp fa-solid fa-check' aria-hidden='true'></i> Available in <strong>Rotch Library</strong> Stacks (NA680.C25 2007)"], |
There was a problem hiding this comment.
Given the assertion, I'm not sure that this is something I'm willing to change. For this test specifically, maybe the full message text isn't necessary (see the following test and its assertions).
|
|
||
| assert_equal('Echo at quebec charlie (delta)', AlmaSru.format_availability(ava_hash)) | ||
| assert_equal( | ||
| "<i class='fa-sharp fa-solid fa-check' aria-hidden='true'></i> Available in <strong>quebec</strong> charlie (delta)", |
There was a problem hiding this comment.
This is the first of three assertions that pins down the full format for the availability message. As such, I'm not sure I'm willing to use less than the full content of the message here. I'm open to counter arguments from a person on this, but the linter alone isn't a big enough signal. If preferred, I'm happy to disable/re-enable this check for this line, however.
| } | ||
|
|
||
| assert_equal( | ||
| "<i class='fa-sharp fa-solid fa-times' aria-hidden='true'></i> Not currently available in <strong>quebec</strong>", |
There was a problem hiding this comment.
This is the second of three assertions that pins down the full format for the availability message. As such, I'm not sure I'm willing to use less than the full content of the message here. I'm open to counter arguments from a person on this, but the linter alone isn't a big enough signal. If preferred, I'm happy to disable/re-enable this check for this line, however.
|
|
||
| assert_equal('Echo at quebec', AlmaSru.format_availability(ava_hash)) | ||
| assert_equal( | ||
| "<i class='fa-sharp fa-solid fa-question' aria-hidden='true'></i> Uncertain availability (Echo) in <strong>quebec</strong>", |
There was a problem hiding this comment.
This is the third of three assertions that pins down the full format for the availability message. As such, I'm not sure I'm willing to use less than the full content of the message here. I'm open to counter arguments from a person on this, but the linter alone isn't a big enough signal. If preferred, I'm happy to disable/re-enable this check for this line, however.
There was a problem hiding this comment.
Pull request overview
This PR restores display of availability statements for Primo (Alma) records by adding an /almasru lookup endpoint backed by the AlmaSru model, then lazy-loading those statements into the Primo result UI via the existing content-loader Stimulus controller.
Changes:
- Added
AlmaController#sruand a new/almasruroute to return availability HTML for a given Alma doc ID. - Replaced Primo API-derived availability rendering with an Alma SRU-triggered lazy-load mechanism in the Primo result partial.
- Extended
content_loader_controller.jswith optional IntersectionObserver-based lazy loading, and refactoredAlmaSruvalidation/extraction + availability formatting.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| test/models/normalize_primo_record_test.rb | Removes tests for Primo-normalized availability fields that are no longer produced. |
| test/models/alma_sru_test.rb | Updates SRU formatting/validation tests to match new HTML-based statements and valid_alma_id?. |
| test/helpers/record_helper_test.rb | Removes tests for the deleted availability helper behavior. |
| test/controllers/alma_controller_test.rb | Adds integration tests for the new /almasru endpoint (but currently has broken status assertions). |
| config/routes.rb | Adds /almasru route and reorders lookup routes. |
| app/views/search/_trigger_almasru.html.erb | Adds a new content-loader trigger partial for Alma SRU availability. |
| app/views/search/_result_primo.html.erb | Uses AlmaSru.valid_alma_id? to decide when to render the SRU trigger. |
| app/views/alma/sru.html.erb | Adds the endpoint’s HTML view rendering availability statements. |
| app/models/normalize_primo_record.rb | Stops emitting :availability and :other_availability in normalized Primo records. |
| app/models/alma_sru.rb | Renames .alma_sru_enabled? to .enabled?, adds valid_alma_id?/extract_alma_id, changes formatting, collapses multiple AVA entries. |
| app/javascript/controllers/content_loader_controller.js | Adds lazyLoading value + IntersectionObserver support. |
| app/helpers/record_helper.rb | Removes the availability/location helpers; leaves icon helper (currently generating invalid HTML). |
| app/controllers/alma_controller.rb | Introduces the controller action serving SRU availability HTML without layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -151,6 +159,37 @@ def self.alma_sru_enabled? | |||
| true | |||
| end | |||
There was a problem hiding this comment.
This is addressed (perhaps not resolved entirely) in the next commit
Update test and teardown for AlmaSru.enabled? Update result_get? check to include Alma ID format We've removed the availability, so that cause and test were meaningless.
| phrase = "#{_phrase_start(ERB::Util.html_escape(availability['e'].to_s))} <strong>#{ERB::Util.html_escape(availability['q'].to_s)}</strong> " \ | ||
| "#{ERB::Util.html_escape(availability['c'].to_s)}".squish | ||
| phrase += " (#{ERB::Util.html_escape(availability['d'].to_s)})" if availability['d'].present? | ||
| phrase |
| phrase = "#{availability['e']&.humanize} at #{availability['q']} #{availability['c']}".squish | ||
| phrase += " (#{availability['d']})" if availability['d'].present? | ||
|
|
||
| phrase = "#{_phrase_start(ERB::Util.html_escape(availability['e'].to_s))} <strong>#{ERB::Util.html_escape(availability['q'].to_s)}</strong> " \ |
This integrates the recently-added
AlmaSrumodel with the UI, restoring the availability statements for users to see. This involves:/almasruwhich invokes the lookup methodIn service of these goals, a few other changes get introduced:
data-content-loader-lazy-loading-value=""to the tag which calls the controller.valid_alma_id?()method. The UI leverages this to make sure we only ask for this lookup when the ID seems relevant (there's no way to know from just an identifier whether a record will actually have an availability block - but we can tell when a record comes from Alma).Side effects:
AlmaSrugets renamed from.alma_sru_enabled?to just.enabled?Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
Do searches in the review app (or when running locally), and watch the network traffic in your dev tools. You should see lazy-loading happening, and you should only see this happening for records with Alma docIDs. This should happen across tabs.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing